Respect tsconfig.json extends when validating config #5537

ianschmitz · 2018-10-23T03:45:53Z

This PR enhances #5532 by adding support for extends in tsconfig.json. We use the TypeScript compiler to parse the configuration, which traverses the relationships.

Some interesting tidbits:

The output from ts.readConfigFile() is mutated after running ts.parseJsonConfigFileContent() which merges in include and exclude from extended config files, but leaves the compilerOptions untouched:

We then use ts.parseJsonConfigFileContent() which gives us a parsed options among other things. For example you can see how module is missing from raw, but is present in options, as it was included by extending tsconfig.test.json:

{
    options: {
        pretty: true,
        target: 1,
        allowJs: true,
        skipLibCheck: true,
        esModuleInterop: true,
        allowSyntheticDefaultImports: true,
        strict: true,
        module: 6,
        moduleResolution: 2,
        resolveJsonModule: true,
        isolatedModules: true,
        noEmit: true,
        jsx: 1,
        configFilePath: undefined,
    },
    fileNames: [
        "C:/Projects/create-react-app/packages/react-scripts/template/src/App.js",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/index.tsx",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/react-app.d.ts",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.d.ts",
        "C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.js",
    ],
    projectReferences: undefined,
    typeAcquisition: { enable: false, include: [], exclude: [] },
    raw: {
        extends: "./tsconfig.test.json",
        compileOnSave: false,
        compilerOptions: {
            target: "es5",
            allowJs: true,
            skipLibCheck: true,
            esModuleInterop: true,
            allowSyntheticDefaultImports: true,
            strict: true,
            moduleResolution: "node",
            resolveJsonModule: true,
            isolatedModules: true,
            noEmit: true,
            jsx: "preserve",
        },
    },
    errors: [],
    wildcardDirectories: {
        "C:/Projects/create-react-app/packages/react-scripts/template/src": 1,
    },
    compileOnSave: false,
    configFileSpecs: {
        filesSpecs: undefined,
        includeSpecs: ["src"],
        excludeSpecs: ["**/__tests__/**", "**/?*test.*", "**/?*spec.*"],
        validatedIncludeSpecs: ["src"],
        validatedExcludeSpecs: [
            "**/__tests__/**",
            "**/?*test.*",
            "**/?*spec.*",
        ],
        wildcardDirectories: {
            "C:/Projects/create-react-app/packages/react-scripts/template/src": 1,
        },
    },
}

Timer · 2018-10-23T04:33:33Z

packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js

@@ -86,8 +67,44 @@ function verifyTypeScriptSetup() {
    process.exit(1);
  }

+  const compilerOptions = {


Why was this object moved? Seems like unnecessary diff noise.

It was done to add in the parsedValue properties, which is using the typescript compiler reference. Alternatively we could leave it in place and hard code the values from typescript. Here's an example:

enum ModuleKind { None = 0, CommonJS = 1, AMD = 2, UMD = 3, System = 4, ES2015 = 5, ESNext = 6 }

What do you think?

Let's not hard code. I'm fine with this move.

Timer · 2018-10-23T04:35:05Z

packages/react-scripts/scripts/utils/verifyTypeScriptSetup.js

+    // Calling this function also mutates the tsconfig above,
+    // adding in "include" and "exclude", but the compilerOptions remain untouched
+    const result = ts.parseJsonConfigFileContent(
+      config,


This should probably be tsconfig for clarity. Kinda sucks that mutation happens, but I'm OK with it.

Actually, we can't mutate this here. If we write out the tsconfig the file will include the include and exclude options which would be unexpected. Can we deep clone tsconfig before passing it to this function?

Ah yes good point about it getting written out! Definitely need to fix that.

Yep we can deep clone it. We will likely need to create a couple extra variables then to hold on to the include and exclude that is mutated on the clone, as below we're relying on tsconfig.include and tsconfig.exclude to check.

Sound good?

I'm not sure I follow. We should be fine to clone the entire thing without special behavior.

All checks are going to be made against this new result object AFAIK. Only changes (no reads) will be made against the original tsconfig.

Hmm yeah let me think through this a bit...

So currently tsconfig is what was read from the tsconfig.json. When we call ts.parseJsonConfigFileContent() it mutates the provided tsconfig, and adds in the include and exclude properties that came in from extended tsconfig config files (I find this to be a strange behavior. Their API for reading the config files isn't the most intuitive). tsconfig.compilerOptions is still the original raw properties from the root tsconfig.json.

parsedOptions contains the compilerOptions that was parsed from reading in all the extended tsconfig.json files.

If we wanted to clone tsconfig before passing it into ts.parseJsonConfigFileContent(), it means we wouldn't know the include and exclude values below when doing the if (tsconfig.include == null) { ... } check, as the include and exclude could be part of one of the extended files as you can see in my screenshot in the top post.

I hope that makes sense. What do you think would be the best approach here?

eps1lon · 2018-10-23T07:05:51Z

Really excited about typescript support in vanilla create-react-app. Great work 👍

It would be nice to include a helpful error message when parsing failed.

You could add

const formatDiagnosticHost = {
  getCanonicalFileName: fileName => fileName,
  getCurrentDirectory: ts.sys.getCurrentDirectory,
  getNewLine: () => os.EOL,
};

- throw error
+ throw new Error(ts.formatDiagnostic(error, formatDiagnosticHost));

- throw result.errors[0]
+ ts.formatDiagnostics(result.errors, formatDiagnosticHost)

and then add that error to console.error. This way you get something like
Error: tsconfig.json(15,21): error TS1002: Unterminated string literal.

Without ts.formatDiagnostic it would only say TypeError: error is not a constructor

If this is out of scope for this PR I can open a separate PR. I already added it locally.

ianschmitz · 2018-10-23T18:20:48Z

Really excited about typescript support in vanilla create-react-app. Great work 👍

It would be nice to include a helpful error message when parsing failed.

You could add
const formatDiagnosticHost = {
  getCanonicalFileName: fileName => fileName,
  getCurrentDirectory: ts.sys.getCurrentDirectory,
  getNewLine: () => os.EOL,
};
- throw error
+ throw new Error(ts.formatDiagnostic(error, formatDiagnosticHost));
- throw result.errors[0]
+ ts.formatDiagnostics(result.errors, formatDiagnosticHost)
and then add that error to console.error. This way you get something like
Error: tsconfig.json(15,21): error TS1002: Unterminated string literal.

Without ts.formatDiagnostic it would only say TypeError: error is not a constructor

If this is out of scope for this PR I can open a separate PR. I already added it locally.

@Timer what do you think about this? Currently we're just throwing the error and catching it without displaying any of the error information to the user. I'm happy to help improve this experience if we think the user should see this info.

Timer · 2018-10-23T18:46:15Z

I'd like to improve the error experience if the above change helps.

Timer · 2018-10-23T19:30:02Z

@ianschmitz I pushed a work around, what do you think?

Timer · 2018-10-23T19:33:24Z

See 1feaa5c

ianschmitz · 2018-10-23T19:49:56Z

@ianschmitz I pushed a work around, what do you think?

Ooo i like that. Thanks for doing that!

@eps1lon would you like to create a PR after this one goes in? I don't want to steal the credit 😛

Timer · 2018-10-23T19:52:27Z

I had added the change for the friendly errors since it was so small, thanks @eps1lon!

eps1lon · 2018-10-23T19:53:55Z

I think everything was covered by 38bd634. If PR credit was so important I would've waited till this get's merged and open then. The sooner TS supports gets release the better.

* Use TS to resolve tsconfig extends * Prevent modifications to original tsconfig * Print friendly error

Use TS to resolve tsconfig extends

f6a1235

facebook-github-bot added the CLA Signed label Oct 23, 2018

Timer added the tag: bug fix label Oct 23, 2018

Timer added this to the 2.1 milestone Oct 23, 2018

Timer suggested changes Oct 23, 2018

View reviewed changes

ianschmitz mentioned this pull request Oct 23, 2018

[WIP] TypeScript final touches #5514

Closed

3 tasks

Prevent modifications to original tsconfig

1feaa5c

Print friendly error

38bd634

Timer merged commit 315ff4b into facebook:master Oct 23, 2018

ianschmitz deleted the parse-tsconfig branch October 23, 2018 19:57

chanand pushed a commit to chanand/create-react-app that referenced this pull request Oct 25, 2018

Respect tsconfig.json extends when validating config (facebook#5537)

a7b928f

* Use TS to resolve tsconfig extends * Prevent modifications to original tsconfig * Print friendly error

Timer added the issue: typescript label Oct 28, 2018

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Oct 29, 2018

Respect tsconfig.json extends when validating config (facebook#5537)

5d76856

* Use TS to resolve tsconfig extends * Prevent modifications to original tsconfig * Print friendly error

nate770 pushed a commit to ONTW/create-react-app that referenced this pull request Oct 30, 2018

Respect tsconfig.json extends when validating config (facebook#5537)

b20f4a3

* Use TS to resolve tsconfig extends * Prevent modifications to original tsconfig * Print friendly error

lock bot locked and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect tsconfig.json extends when validating config #5537

Respect tsconfig.json extends when validating config #5537

ianschmitz commented Oct 23, 2018

Timer Oct 23, 2018

ianschmitz Oct 23, 2018

Timer Oct 23, 2018

Timer Oct 23, 2018

Timer Oct 23, 2018

ianschmitz Oct 23, 2018

Timer Oct 23, 2018

ianschmitz Oct 23, 2018

eps1lon commented Oct 23, 2018

ianschmitz commented Oct 23, 2018

Timer commented Oct 23, 2018

Timer commented Oct 23, 2018

Timer commented Oct 23, 2018

ianschmitz commented Oct 23, 2018

Timer commented Oct 23, 2018

eps1lon commented Oct 23, 2018

Respect tsconfig.json extends when validating config #5537

Respect tsconfig.json extends when validating config #5537

Conversation

ianschmitz commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Oct 23, 2018

ianschmitz commented Oct 23, 2018

Timer commented Oct 23, 2018

Timer commented Oct 23, 2018

Timer commented Oct 23, 2018

ianschmitz commented Oct 23, 2018

Timer commented Oct 23, 2018

eps1lon commented Oct 23, 2018